Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use com_google_protobuf in WORKSPACE #2432

Merged
merged 14 commits into from
Nov 21, 2024

Conversation

comius
Copy link
Contributor

@comius comius commented Nov 20, 2024

The only way to support both workspace and bzlmod mode, is to call protobuf com_google_protobuf. This is because old Bazel's encode it in default values of --protoco_compiler flag, and so new Bazel 8 needs to do the same.

For bzlmod, upgrade rules_cc to 0.0.16 and rules_java (dev dep) to 8.3.1. Those are minimal versions that are also calling protobuf again com_google_protobuf.

For workspace, upgrade rules_cc to 0.1.0. This is an incompatible version that doesn't call Protobuf. rules_python users may use it. In case they need cc_proto_library in @rules_cc//cc/defs.bzl, they can overwrite the version to 0.0.16 in WORKSPACE (or use protobuf_deps that already does that).

Disable docs generation targets on WORKSPACE CI setups. They are broken by rules_java upgrade.

Upgrades dependencies:

  • rules_cc 0.0.16 (Bzlmod) and rules_cc 0.1.0 (WORKSPACE)
  • rules_java 8.3.1
  • bazel_skylib 1.7.0 (workspace; bzlmod already specifying that version)
  • protobuf 29.0-rc2 (workspace; bzlmod already specifying that version)

Fixes #2429

@comius
Copy link
Contributor Author

comius commented Nov 20, 2024

Hey @rickeylev, this should now also support WORKSPACE mode better in combination with autoloaded rules.

Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update changelog. I've pushed a change for the deps part.

Can you add something under Fixed for what it fixes? I get that the repo name being different might cause issues, but don't entirely understand what broke/why (e.g. our CI is green). Is it that the broader WORKSPACE ecosystem has settled more on com_google_protobuf as the name? Or this only shows up in Bazel 9, or something?

.bazelci/presubmit.yml Outdated Show resolved Hide resolved
@aignas
Copy link
Collaborator

aignas commented Nov 21, 2024

Does this also fix #2429 ?

@aignas aignas added this to the v1.0.0 milestone Nov 21, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
@rickeylev
Copy link
Collaborator

Does this also fix #2429 ?

Yes, I think so. Added to the description.

@comius
Copy link
Contributor Author

comius commented Nov 21, 2024

Can you add something under Fixed for what it fixes? I get that the repo name being different might cause issues, but don't entirely understand what broke/why (e.g. our CI is green). Is it that the broader WORKSPACE ecosystem has settled more on com_google_protobuf as the name? Or this only shows up in Bazel 9, or something?

Will add to Fixed.

The reason for @com_google_protobuf is much simpler. Bazel 6 and 7 have the name hardcoded in the default values of flags, like --protocol_compiler. To have continuity of WORKSPACE mode with Bazel 8, we need to keep the same name. Once we're able to drop the WORKSPACE mode, Bzlmod doesn't care how protobuf is called (the name is correctly resolved in bzlmod mode even when coming from a flag).

Things are even worse. Bazel 6 and 7 call @rules_cc//cc:defs.bzl from @bazel_tools. So they trigger the need for @protobuf even if you don't do anything. (Bazel 8 doesn't use defs.bzl). I also can't remove cc_proto_library from @rules_cc//cc:defs.bzl, because some major repositories like grpc depends on it.

@comius comius force-pushed the use-com_google_protobuf branch from b215c76 to bcbd4e0 Compare November 21, 2024 12:37
comius and others added 8 commits November 21, 2024 13:57
@comius comius force-pushed the use-com_google_protobuf branch from bcbd4e0 to 1e43ae6 Compare November 21, 2024 13:09
@rickeylev
Copy link
Collaborator

re: docs building:

Thanks for noticing that. We just need our docs to build for (ubuntu + bzlmod + current bazel) (they shouldn't have been building for other configs). Having it work for macs is only needed for local development. I've push some changes to modify the target_compatible_with settings so docs skip unsupported cases.

//sphinxdocs looks OK, too, now.

This should remove the need for changing the CI configs. I've pushed a change to that effect; lets see what CI says.

@rickeylev rickeylev added this pull request to the merge queue Nov 21, 2024
Merged via the queue into bazelbuild:main with commit e6a6a3a Nov 21, 2024
4 checks passed
@comius
Copy link
Contributor Author

comius commented Nov 22, 2024

Update on docs building:

rules_java 8.5.1 will contain a fix, so a part of this PR could be undone by another upgrade (IMO it's better to build docs only on one platform)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bazel CI] Error: no native function or rule 'cc_proto_library'
3 participants